Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update http proxy to handle client disconnect scenario #10688

Merged
merged 11 commits into from
Jan 9, 2025

Conversation

liliankasem
Copy link
Member

@liliankasem liliankasem commented Dec 10, 2024

Issue describing the changes in this PR

resolves #10600

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

Additional information

  • Introduce HttpForwardingException as this makes more sense than an invalid operation when a forwarding error occurs
  • Update the proxy service to check for client disconnect and log a helpful message instead of throwing an exception
  • Introduce HandleCancellationMiddleware to return Status499ClientClosedRequest on client disconnect
  • Update RetryProxyHandler to stop retries when the request is cancelled

Testing

See comments below for test scenarios completed. Behaviour change from this PR have been compared to a) what is in dev today and b) what aspnet does

@jviau
Copy link
Contributor

jviau commented Dec 11, 2024

For the scenarios you tested, can you compare how this would behave in a regular AspNetCore app? I think we should align with that behavior as best we can. If client disconnects, is an exception thrown? Is the request considered failed or successful?

@liliankasem
Copy link
Member Author

For the scenarios you tested, can you compare how this would behave in a regular AspNetCore app? I think we should align with that behavior as best we can. If client disconnects, is an exception thrown? Is the request considered failed or successful?

Here's what I found using a plain aspnet app:

RequestAborted

  • Not utilizing the CT: work gets to complete without interuption, no errors or exceptions; no response sent as client does not exist
  • Utilizes CT, but not catching exception: work stops, no exception is seen (handled by the framework maybe?); no response sent as client does not exist
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and is handled by user code, no errors or exceptions; no response sent as client does not exist

CancellationTokenSource cancelled

  • Not utilizing the CT: work gets to complete without interuption, no errors or exceptions; successful completion with user defined status code
  • Utilizes CT, but not catching exception: TaskCancelled exception is thrown; fails with 500 internal server error
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and handled by user code, no errors or exceptions; successful completion with user defined status code

Here's the app I used, or do you think I need to try and replicate what we're doing with the YARP as well?

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/process", async (HttpContext context) =>
{
    var cancellationToken = context.RequestAborted; // Client disconnect
    // var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(5)).Token; // Cancel after 5 seconds

    try
    {
        // Simulate some work that can be cancelled
        Console.WriteLine("Starting work...");
        for (int i = 0; i < 10; i++)
        {
            // Simulate work
            await Task.Delay(1000, cancellationToken);
            Console.WriteLine($"Work iteration {i + 1} completed.");
        }
        Console.WriteLine("Work completed.");

        context.Response.StatusCode = StatusCodes.Status200OK;
        await context.Response.WriteAsync("Processing completed successfully.");
    }
    catch (OperationCanceledException)
    {
        Console.WriteLine("Request was cancelled.");
        context.Response.StatusCode = StatusCodes.Status499ClientClosedRequest; // Status code for client closed request
        await context.Response.WriteAsync("Request was cancelled.");
    }
});

app.Run();

@liliankasem liliankasem marked this pull request as ready for review December 12, 2024 23:23
@liliankasem liliankasem requested a review from a team as a code owner December 12, 2024 23:23
@jviau
Copy link
Contributor

jviau commented Dec 16, 2024

Don't need to test with YARP directly, but I would like us to compare with your found results for AspNetCore with what happens in the functions YARP scenario today.

Also: StatusCodes.Status499ClientClosedRequest - good to know there is a specific HTTP code for that! Do you know if we set this status code in functions host for client disconnect scenarios?

@liliankasem
Copy link
Member Author

liliankasem commented Dec 17, 2024

Don't need to test with YARP directly, but I would like us to compare with your found results for AspNetCore with what happens in the functions YARP scenario today.

Also: StatusCodes.Status499ClientClosedRequest - good to know there is a specific HTTP code for that! Do you know if we set this status code in functions host for client disconnect scenarios?

@jviau Status499ClientClosedRequest is technically an unofficial status code, and no, we don't use it anywhere in the host/

In comparison, here is the result to the exact same code as above but in a function app with what we have today in dev:

RequestAborted

  • Not utilizing the CT: work gets to complete without interuption, DefaultHttpProxyService retry handler throws exception, function invocation fails with forwarder error from host, status code 500, no response to client
    • Exception the moment we cancel: Unsupported exception type in RetryProxyHandler. Request will not be retried. Exception: System.Threading.Tasks.TaskCanceledException: The operation was canceled.
    • Exception after the function completes: Microsoft.Azure.WebJobs.Host.FunctionInvocationException: Exception while executing function: Functions.MyHttpTriggerNoCT - Failed to proxy request with ForwarderError: RequestCanceled
  • Utilizes CT, but not catching exception: work stops, DefaultHttpProxyService retry handler throws exception, function invocation fails with exception, status code 500, no response to client
    • Exception the moment we cancel: Unsupported exception type in RetryProxyHandler. Request will not be retried. Exception: System.Threading.Tasks.TaskCanceledException: The operation was canceled.
    • User code gets unhandled exception: Microsoft.Azure.WebJobs.Script.Workers.Rpc.RpcException: Result: Function 'MyHttpTriggerCTNoCatch', Invocation id 'e33ab37a-069f-4a89-b64c-2e7152946142': An exception was thrown by the invocation.
    • Exception after the function completes: Microsoft.Azure.WebJobs.Host.FunctionInvocationException: Exception while executing function: Functions.MyHttpTriggerNoCT - Failed to proxy request with ForwarderError: RequestCanceled
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and is handled by user code, DefaultHttpProxyService retry handler throws exception, function invocation fails with forwarder error from host, status code 500, no response to client
    • Exception the moment we cancel: Unsupported exception type in RetryProxyHandler. Request will not be retried. Exception: System.Threading.Tasks.TaskCanceledException: The operation was canceled.
    • Exception after the function completes: Microsoft.Azure.WebJobs.Host.FunctionInvocationException: Exception while executing function: Functions.MyHttpTriggerNoCT - Failed to proxy request with ForwarderError: RequestCanceled

CancellationTokenSource cancelled

  • Not utilizing the CT: work gets to complete without interuption, no errors or exceptions, function invocation successful with user defined status code
  • Utilizes CT, but not catching exception: work stops, TaskCancelled exception is thrown, function invocation fails with unhandled cancellation exception, status code 500 returned to client
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and handled by user code, no errors or exceptions; function invocation successful with user defined status code

And with my changes in this PR:

RequestAborted

  • Not utilizing the CT: work gets to complete without interruption, function invocation successful, status code 499 (our http logs/trace), no response to client
    • New Log: The client disconnected while the function was processing the request.
  • Utilizes CT, but not catching exception: work stops, TaskCancelled exception is thrown, function invocation fails with unhandled exception, status code 499, no response to client
    • User code gets unhandled exception: Microsoft.Azure.WebJobs.Script.Workers.Rpc.RpcException: Result: Function 'MyHttpTriggerCTNoCatch', Invocation id 'e33ab37a-069f-4a89-b64c-2e7152946142': An exception was thrown by the invocation.
    • New Log: The client disconnected while the function was processing the request.
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and is handled by user code, function invocation successful, status code 499, no response to client
    • New Log: The client disconnected while the function was processing the request.

CancellationTokenSource cancelled

  • Not utilizing the CT: work gets to complete without interruption, no errors or exceptions, function invocation successful with user defined status code
  • Utilizes CT, but not catching exception: work stops, TaskCancelled exception is thrown, function invocation fails with unhandled exception, status code 500 returned to client
  • Utilizes CT & catches exceptions: OperationCanceledException is thrown and handled by user code, no errors or exceptions; function invocation successful with user defined status code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultHttpProxyService does not take into account cancellation (ForwarderError.RequestCanceled)
3 participants